Fix DRA nodeSelector for in-tree driver mode#1851
Conversation
When spec.moduleLoader is nil (in-tree mode), the DRA reconciler's nodeSelector requires the KMM ready label, which is never set because no module loading occurs. Fall back to spec.selector instead, matching the existing behavior in the device plugin reconciler. Signed-off-by: Anthony Byrne <abyrne@redhat.com>
✅ Deploy Preview for openshift-kmm ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomerNewman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThe DRA reconciler's ChangesDRA nodeSelector fallback fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controllers/dra_reconciler.go (1)
552-553: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider defensive copy of
mod.Spec.Selectorto avoid shared map reference.When
ModuleLoaderis nil,nodeSelector = mod.Spec.Selectorassigns the same underlying map to both the Module spec and the DaemonSet'sNodeSelector. While no modifications occur after this point in the current code, a future change that mutates the DaemonSet'sNodeSelectorwould silently corruptmod.Spec.Selector. This pattern is consistent withdevice_plugin_reconciler.go, so it's not a blocking concern.♻️ Optional defensive copy
} else { - nodeSelector = mod.Spec.Selector + nodeSelector = make(map[string]string, len(mod.Spec.Selector)) + for k, v := range mod.Spec.Selector { + nodeSelector[k] = v + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controllers/dra_reconciler.go` around lines 552 - 553, Defensively copy mod.Spec.Selector before assigning it to nodeSelector in the DRA reconciler fallback path, since the current direct assignment shares the underlying map with the Module spec. Update the branch in the reconciler that handles the nil ModuleLoader case so the DaemonSet’s NodeSelector gets its own copy, matching the safer pattern used in device_plugin_reconciler.go and avoiding accidental mutation of mod.Spec.Selector.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/controllers/dra_reconciler.go`:
- Around line 552-553: Defensively copy mod.Spec.Selector before assigning it to
nodeSelector in the DRA reconciler fallback path, since the current direct
assignment shares the underlying map with the Module spec. Update the branch in
the reconciler that handles the nil ModuleLoader case so the DaemonSet’s
NodeSelector gets its own copy, matching the safer pattern used in
device_plugin_reconciler.go and avoiding accidental mutation of
mod.Spec.Selector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a086002e-8ffe-482f-b438-0cb9c95b4976
📒 Files selected for processing (2)
internal/controllers/dra_reconciler.gointernal/controllers/dra_reconciler_test.go
|
/lgtm |
openshift-merge-bot
Bot
merged commit d27863e
into
rh-ecosystem-edge:main
4 participants
When spec.moduleLoader is nil (in-tree mode), the DRA reconciler's nodeSelector requires the KMM ready label, which is never set because no module loading occurs. Fall back to spec.selector instead, matching the existing behavior in the device plugin reconciler.
/cc @ybettan @yevgeny-shnaidman
/assign @ybettan @yevgeny-shnaidman
fix #1850
Summary by CodeRabbit